Conversation
spitsfire
left a comment
spitsfire
left a comment
There was a problem hiding this comment.
Nicely done, Amber! Perhaps during a break week, you can finish up Wave 6.
Things to consider:
- Keep your responses as consistent and predictable as possible. If you start with
jsonify, then use it all the way through (where possible). - Don't forget your status codes. Several of them are missing form your responses.
- There are some places we could pull out code into helper functions in order to keep routes shorter.
| is_valid_goal = "title" in request_body | ||
| if not is_valid_goal: |
There was a problem hiding this comment.
This works, but it is more Pythonic to combine this:
if "title" not in request_body:| request_body = request.get_json() | ||
| is_valid_goal = "title" in request_body | ||
| if not is_valid_goal: | ||
| abort(make_response({"details": "Invalid data"}, 400)) |
There was a problem hiding this comment.
How could we make this error message more descriptive for the user to understand what is missing? Maybe we can say that title is missing or something!
| goals = Goal.query.all() | ||
|
|
||
| goals_response = [goal.goal_to_dict() for goal in goals] | ||
| return jsonify(goals_response) |
There was a problem hiding this comment.
Oops, don't forget your status code! Technically, it will add one by default, but consider being explicit and always adding the status code.
| goals_response = [goal.goal_to_dict() for goal in goals] | ||
| return jsonify(goals_response) | ||
|
|
||
| @goal_bp.route("/<goal_id>", methods=["GET"]) |
| response_body = { | ||
| "goal": goal.goal_to_dict() | ||
| } | ||
| return response_body |
There was a problem hiding this comment.
Careful here! While this does work, it is very different form the previous routes you've done. Try to keep them as consistent as possible. If you used jsonfy in a previous route response, do it for all of them (with the few exceptions that there may be)
| response_body = { | ||
| "task": task.task_to_dict() | ||
| } | ||
| return response_body |
There was a problem hiding this comment.
Try to keep your responses as consistent as possible. Since you've used jsonify previously, continue using it throughout.
return jsonify(response_body), 200| response_body = { | ||
| "task": task.task_to_dict() | ||
| } | ||
| return jsonify(response_body) |
There was a problem hiding this comment.
Oops, you forgot your status code! It will implicitly return a status code, but we should add it ourselves to be in control of what is returned
| SLACK_BOT_TOKEN = os.environ.get("SLACK_BOT_TOKEN") | ||
| my_headers = { | ||
| "Authorization": f"Bearer {SLACK_BOT_TOKEN}" | ||
| } | ||
|
|
||
| data_payload = { | ||
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
|
|
||
| requests.post("https://slack.com/api/chat.postMessage", | ||
| headers=my_headers, | ||
| data=data_payload) |
There was a problem hiding this comment.
This would be a good candidate for a helper function
| return jsonify(response_body) | ||
|
|
||
|
|
||
| # ~~Below is the the code I would have used given Slack documentation for calling their API~~ |
There was a problem hiding this comment.
Good job working with the documentation to find a solution! 👍
| response_body = abort(make_response({"message": f"{task_id} not found"}, 404)) | ||
| return response_body |
There was a problem hiding this comment.
I believe abort has an implicit return, so we can combine these two lines together:
abort(make_response({"message": f"{task_id} not found"}, 404))2 participants
No description provided.